Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Require action types to be strings #4544

Merged
merged 4 commits into from
May 9, 2023
Merged

Require action types to be strings #4544

merged 4 commits into from
May 9, 2023

Conversation

EskiMojo14
Copy link
Contributor

@EskiMojo14 EskiMojo14 commented May 8, 2023 •
edited
Loading

name: 🐛 Bug fix or new feature
about: Require action types to be strings

PR Type

Does this PR add a new feature, or fix a bug?

It prevents passing non-string action types to store.

Why should this PR be included?

Per #4129 (comment) :

we've always said they should be serializable, and ought to be strings.

Someone over on Twitter said they'd once tried to use Symbols to make them truly unique, but tbh slice namespacing gives you like 95% uniqueness anyway, so I don't see that as a reason to continue to allow them. As mentioned above, TS enums can have number values, but again why would you do that? We want them to be readable in the DevTools.

So, given that we've always effectively said they should be strings, we might as well enforce it.

Checklist

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Is there an existing issue for this PR?
  • Have the files been linted and formatted?
  • Have the docs been updated to match the changes in the PR?
  • Have the tests been updated to match the changes in the PR?
  • Have you run the tests locally to confirm they pass?

Bug Fixes

What is the current behavior, and the steps to reproduce the issue?

Non-string action types are allowed to be dispatched to store.

What is the expected behavior?

Action types should be enforced as string

How does this PR fix the problem?

Store throws an error if action type is not string.

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 8, 2023 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2952422:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration

@EskiMojo14 EskiMojo14 changed the title String action type Require action types to be strings May 9, 2023
@EskiMojo14 EskiMojo14 marked this pull request as ready for review May 9, 2023 00:01
@netlify
Copy link

netlify bot commented May 9, 2023

✅ Deploy Preview for redux-docs ready!

Name Link
🔨 Latest commit 2952422
🔍 Latest deploy log https://app.netlify.com/sites/redux-docs/deploys/64598d211409780008b267fc
😎 Deploy Preview https://deploy-preview-4544--redux-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@markerikson
Copy link
Contributor

Heh, technically the docs update will apply now, but hey, why not :)

@markerikson markerikson merged commit eaa0ffc into reduxjs:master May 9, 2023
@EskiMojo14 EskiMojo14 deleted the string-action-type branch May 9, 2023 15:36
@EskiMojo14
Copy link
Contributor Author

yeah, technically there's no harm in the docs saying that action types must be strings early 😄

@EskiMojo14 EskiMojo14 linked an issue May 9, 2023 that may be closed by this pull request
Actually require that action.type must be a string #4543
Closed
This was referenced Aug 9, 2024
Open
Open
Open
Open
@lucentlabz lucentlabz mentioned this pull request Sep 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timdorr timdorr timdorr left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Actually require that action.type must be a string
3 participants
@EskiMojo14 @markerikson @timdorr